Skip to content

Cleanup: BDD CI speedup, web-ui quoting, binary-upgrade docs, NO_COLOR, EL7/EL8 RPM - #251

Merged
vadv merged 11 commits into
masterfrom
fix/bdd-web-ui-quoting
May 13, 2026
Merged

vadv merged 11 commits into
masterfrom
fix/bdd-web-ui-quoting

Conversation

@vadv

@vadv vadv commented May 13, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Cleanup pass on the BDD CI pipeline that surfaced after merging #250, plus two operator-facing fixes that landed in the same review window.

What changes

  • BDD: Web UI was failing on nine should contain assertions about /api/auth/config and /api/config. The cucumber-rs {string} parameter parser does not unescape \" inside double-quoted Gherkin strings, so the steps were searching the curl output for literal \"key\":\"value\" substrings with backslashes that never appear in the JSON body. Swap the outer Gherkin quotes to single quotes so the embedded " pass through verbatim.
  • prebuild-bdd job + actions/cache@v4 on target/ and .cargo/. Each entry in the 23-way bdd-tests matrix used to start from a cold workspace and pay a full cargo download + compile cycle (~5-10 minutes per suite) inside the test-runner image before the actual scenarios ran. The new job compiles the BDD binaries once for default features and once for --features tls-migration; the matrix restores the same cache and reuses the prebuilt artifacts. The cache had to be chowned back to the runner user after each docker run, otherwise actions/cache@v4 was silently saving an empty tarball (root-owned files unreadable to uid 1001).
  • bdd-tests matrix max-parallel: 6 → 12. With the cache, each entry is now link-time + scenario, not full compile, so the CPU pressure that drove the previous cap is gone.
  • docker login ghcr.io is wrapped in nick-fields/retry@v3 in every step that needed it (check-image, build-and-push-image, prebuild-bdd, bdd-tests matrix). docker/login-action@v3 calls docker login once; ghcr.io's auth handshake occasionally times out under GHA load (Client.Timeout exceeded while awaiting headers) and was killing whole jobs.
  • tutorials/binary-upgrade.md (EN+RU): cp pg_doorman_new /usr/bin/pg_doorman → install -m 0755. cp rewrites the inode the running pg_doorman is mapped from and risks SIGBUS/SIGSEGV mid-upgrade; install writes to a tempfile and renames. systemd unit example flipped from Type=simple to Type=notify with NotifyAccess=all, ExecReload=SIGHUP, Restart=on-failure, RestartSec=5s, LimitNOFILE=1048576, User=pg_doorman, KillMode=mixed, TimeoutStopSec=120. Quick-start kill -USR2 $(pgrep -f ...) replaced with systemctl kill -s SIGUSR2; verification with systemctl show -p MainPID. Apt/dnf package install added as the preferred path.
  • TextLogger auto-disables ANSI colours when stderr is not a TTY and on NO_COLOR. Under systemd the unit's stderr was the journal pipe; the colour escapes were leaking into MESSAGE and journalctl rendered every record as [NNN blob data]. The clap env attribute on --no-color also made NO_COLOR=1 crash startup; dropped, and the logger now reads NO_COLOR directly per https://no-color.org/.
  • EL7/EL8 RPM build no longer fails on perl-FindBin / perl-IPC-Cmd modular filtering. The CI workflow bundles the pure-Perl modules (FindBin + IPC::Cmd dependency closure) into perl_modules.tar.gz as Source3; the spec extracts it and exports PERL5LIB so the openssl-sys Configure path resolves the modules regardless of distro, matching the offline-tarball pattern we already use for the Rust toolchain.

Risk

  • All feature/scenario edits are test-only. No product behaviour changes.
  • Cache layout uses ${{ runner.os }}-bdd-cargo-${{ image-tag }}-${{ hashFiles('Cargo.lock', 'src/**', 'tests/**', 'build.rs') }} with restore-keys falling back first to any prebuild for the current image tag, then to any prebuild on this OS. GHA cache limit is 10 GB; debug target/ compresses to ~500-800 MB.
  • --no-color: NO_COLOR=false now disables colour (was: colour on). Conforms to no-color.org. Operator surface stays the same: --no-color flag, or any non-empty NO_COLOR.
  • Perl modules tarball ships pure-Perl .pm files only; no XS, portable across EL7/EL8/EL9/Fedora/Amazon Linux.

Test plan

  • BDD: Web UI green on this branch.
  • prebuild-bdd shows Cache restored on the second run with the same Cargo.lock/src/tests hash; matrix entries finish noticeably faster than master.
  • actions/cache save step takes more than the previous ~13 s (saving a populated cache, not an empty tarball).
  • Publish to Fedora COPR PR-stage test-rpm-build matrix is green and Upload to Fedora COPR succeeds on the next release tag for all 18 chroots (including rhel-8, epel-8, centos-stream-8, epel-7).

dmitrivasilyev added 2 commits May 13, 2026 14:12
… web-ui

cucumber-rs `{string}` parameter parser does not unescape `\"` inside double-quoted Gherkin strings, so the nine `should contain` assertions on `/api/auth/config` and `/api/config` were searching the curl output for literal `\"key\":\"value\"` substrings with backslashes that never appear in the JSON body. The first three Web UI scenarios (`sso_enabled`, `current_user`, `role`/`source`, four bind-address keys, `changeable`) fail in BDD: Web UI of the post-merge run because of this.

Swap the outer Gherkin quotes to single quotes so the embedded double quotes pass through verbatim and the substring search hits the JSON body. Scenario semantics are unchanged.
…matrix

Each of the 23 entries in the `bdd-tests` matrix used to start from a cold workspace, run `cargo download` (1-2 min) and recompile `pg_doorman` plus the BDD test crate (4-9 min) inside the test-runner image before the actual tags ran. The fan-out wasted ~150 CPU-min of duplicate compile per CI run and stretched the wall clock to ~30 minutes even at 6 parallel suites.

Add a `prebuild-bdd` job between `prepare-tests` and `bdd-tests`. It checks out the source, restores the cache, pulls the same test-runner image, and compiles `cargo test --test bdd --no-run`, `cargo test --test patroni_proxy_bdd --no-run`, and the `--features tls-migration` variant. `CARGO_HOME=/workspace/.cargo` keeps the registry and the build artefacts inside `github.workspace`, so `actions/cache@v4` stores `.cargo/` and `target/` without touching runner-level paths.

Each matrix entry now `needs: prebuild-bdd`, restores the same cache, and reuses the prebuilt binaries. cargo resolves the test against the cached `target/`, so the per-suite cost drops to a link plus the scenario runtime.
@vadv vadv changed the title test(bdd): fix Gherkin quoting for JSON substring assertions in web-ui test(bdd): fix Gherkin quoting in web-ui and prebuild BDD binaries for cache reuse May 13, 2026
dmitrivasilyev added 8 commits May 13, 2026 14:55
…unit to Type=notify

`cp pg_doorman_new /usr/bin/pg_doorman` rewrites the live inode the running pg_doorman is memory-mapped from. The process can take SIGBUS or SIGSEGV mid-upgrade because its text segment is being overwritten in place. `install -m 0755` writes to a temporary file and renames it onto the target, so the running inode survives and only new processes pick up the new file.

The shipped `pg_doorman.service` already uses `Type=notify`, but the tutorial example showed `Type=simple`. With `Type=simple` systemd marks the unit `active` immediately after exec without waiting for `READY=1`, and the new process spawned by `SIGUSR2` cannot hand off `MainPID` to itself. `Type=notify` + `NotifyAccess=all` lines up the tutorial with the real readiness contract: pg_doorman sends `READY=1` once listeners are bound and `MAINPID=<new_pid>` during an upgrade, both via `sd_notify`. `ExecReload=SIGHUP` covers config reload; `kill -USR2` and `UPGRADE;` still drive the binary upgrade.
…rial

Four operator-facing issues left over after the cp/Type=notify fix:

- `kill -USR2 $(pgrep -f /usr/bin/pg_doorman)` matches command line and signals every pg_doorman process on the host. On a multi-tenant box or after a stuck previous upgrade the cleanup signal lands on neighbours. Switch the Quick start to `systemctl kill -s SIGUSR2 pg_doorman.service`, which targets the single tracked MainPID, and add a stand-alone tip for the PID-file path.

- Verification step relied on `pgrep -f /usr/bin/pg_doorman`, which cannot tell the old PID from the new one once both are alive during the drain window. Replace it with `systemctl show -p MainPID --value`, the same field `Type=notify` updates from `MAINPID=<new_pid>`.

- The shipped systemd example in "Daemon vs foreground" lacked the production knobs every operator ends up adding: `Restart=on-failure` + `RestartSec=5s` to absorb a single crash without a tight loop, `LimitNOFILE=1048576` to clear the default 1024 fd cap that connection-heavy poolers hit first, `User=pg_doorman`/`Group=pg_doorman` for a non-root listener, `KillMode=mixed` so SIGUSR2 reaches the parent and the children, and `TimeoutStopSec=120` aligned with `shutdown_timeout`. Each setting has a one-line comment explaining why it is there.

- Operational checklist still asked operators to verify `ExecReload=/bin/kill -SIGUSR2 $MAINPID`. The new unit uses SIGHUP for reload; SIGUSR2 is the binary-upgrade trigger and lives outside `ExecReload`. Updated the checklist item to match.

- Added a tip above Quick start naming `apt-get install --only-upgrade pg-doorman` / `dnf upgrade pg-doorman` as the preferred path when a package is in scope, so direct-binary `install -m 0755` is positioned as the fallback rather than the recommendation.

EN and RU tutorials updated in lock-step.
…onour NO_COLOR

`TextLogger` writes `\x1b[31m`-style colour escapes to stderr whenever `args.no_color` is false. Under systemd that stderr is the journal pipe, not a terminal, so the escape bytes ended up inside the `MESSAGE` field and journalctl rendered every record as `[NNN blob data]`. Operators reading `journalctl -u pg-pooler` could not see the actual log text without remembering to pass `--no-color` or pin `Environment=NO_COLOR=...`.

`clap`'s `env` attribute on the bool flag also made `NO_COLOR=1` (the canonical https://no-color.org/ value) crash startup because the parser only accepts `true`/`false`.

Wire `should_use_color` between args and `TextLogger::new`. The pure `resolve_color(no_color_flag, env_no_color, stderr_is_tty)` predicate now gates the colour escapes on all three signals: explicit `--no-color`, any non-empty `NO_COLOR` value, and `stderr.is_terminal()`. The four cases of that predicate are covered by unit tests. Drop the `env` attribute on `args.no_color` so a `NO_COLOR=1` env var no longer fails clap parsing; the logger reads the standard's spelling directly via `var_os`.
…no longer fails the whole job

`docker/login-action@v3` calls `docker login` exactly once. When ghcr.io's auth endpoint times out mid-handshake (`Error response from daemon: Get \"https://ghcr.io/v2/\": net/http: request canceled (Client.Timeout exceeded while awaiting headers)`) the entire BDD job, the prebuild job, or the image-check job dies before its real work starts. Outer retries on `docker pull` already handle the pull leg of the same flake, but the login step was a single point of failure.

Replace every `Log in to Container Registry` step in `bdd-tests.yml` with `nick-fields/retry@v3` wrapping a plain `docker login ... --password-stdin`. Three attempts with a 10-second back-off match the policy we already use for `docker pull`, and the credential is passed via stdin so it never appears on the command line or in the process listing.
…logger behaviour

After the previous commit dropped the `env` attribute on `--no-color` and added auto-TTY detection plus `NO_COLOR` handling, the `--help` snapshots in `basic-usage.md` (EN and RU) still showed `disable colors in the log output [env: NO_COLOR=]`. Update the snapshot to the new help text and expand the option description to call out the auto-disable triggers (non-TTY stderr, non-empty `NO_COLOR`).
… COPR builds

`pg-doorman.spec` listed `perl-FindBin` and `perl-IPC-Cmd` as `BuildRequires`. On RHEL/CentOS Stream 7 and 8 these modules ship only inside the modular `perl:5.30` stream, which COPR's mock filters out by default ("package perl-FindBin-... is filtered out by modular filtering"). COPR builders have no network access, so we cannot `dnf module enable perl` inside the chroot, and the rhel-8, centos-stream-8, epel-8, and epel-7 chroots fail at `dnf builddep` before pg_doorman compiles. AlmaLinux 9 / Rocky 9 / Fedora / Amazon Linux 2023 are unaffected because they expose the same modules outside modular streams.

Apply the same offline-tarball pattern we already use for Rust:

- The CI prepare-srpm step now installs `perl-FindBin` and `perl-IPC-Cmd` in the Fedora container, copies the `.pm` files from `@INC` into `perl_modules/`, and tarballs the directory as `perl_modules.tar.gz`.
- The spec adds `Source3: perl_modules.tar.gz`, swaps the two missing `BuildRequires` for `perl-interpreter`, extracts the tarball into `_builddir/perl_modules`, and exports `PERL5LIB="_builddir/perl_modules:$PERL5LIB"` in `%build` so the openssl-sys Configure path resolves `use FindBin` and `use IPC::Cmd` from the bundle on every chroot, modular filtering or not.

The pure-Perl module list covers FindBin plus the dependency closure IPC::Cmd actually walks at use-time (Module::Load::Conditional, Module::Load, Module::Metadata, Params::Check, Locale::Maketext::Simple, ExtUtils::MakeMaker, version, IPC::Open3). Modules without architecture-specific XS bits, so the tarball is portable across the EL7/EL8/EL9/Fedora/Amazon mix.
…s saving an empty tarball

`prebuild-bdd` runs `cargo test --no-run` inside the test-runner container. The container runs as root, so the `target/` and `.cargo/` files it produces are root-owned on the host. `actions/cache@v4`'s post-step runs as the runner user (`uid 1001`) and silently fails to read the root-owned files; in the recent PR #251 runs it saved a roughly empty tarball in ~13 s and the next run's `Restore cargo cache` step finished in 0 s with nothing to restore. Result: every `prebuild-bdd` paid full 5-7 minutes of cargo build even though the cache key matched.

Add a `Chown cargo cache so actions/cache can read it` step after the two docker compile steps. `sudo chown -R "$(id -u):$(id -g)" .cargo target` is enough on GitHub-hosted ubuntu-latest (sudo is available without password) to flip ownership back to the runner user before the cache post-step tars the directories. With the chown the save step takes its real time (single-digit minutes on a fully-built target), and the next run's restore actually populates `.cargo/` and `target/`.
With prebuild-bdd populating the cache, each matrix entry is now bound by link time and the scenario itself (~1-2 min) rather than the full cargo build (~7-9 min). The CPU contention that drove the previous cap of 6 (sleep-heavy lifecycle and SCRAM passthrough reconnect losing their timing margin) does not apply to the link-only path, so we can safely double the fan-out. Wall clock on a green PR roughly halves; the timing-sensitive suites keep their `nick-fields/retry@v3` outer policy.
@vadv vadv changed the title test(bdd): fix Gherkin quoting in web-ui and prebuild BDD binaries for cache reuse Cleanup: BDD CI speedup, web-ui quoting, binary-upgrade docs, NO_COLOR, EL7/EL8 RPM May 13, 2026
…pattern

User flagged that their production unit differs from the tutorial. Adjust the example so it matches what is actually deployed, while keeping the comments load-bearing.

- `NotifyAccess=exec` instead of `all`: the upgrade child is spawned via Command::spawn (fork + execve), so exec scope is enough; `all` was an over-broad surface.
- `ExecReload=/bin/kill -SIGUSR2 $MAINPID` instead of SIGHUP: `pg_doorman -t` runs first under SIGUSR2 and aborts the reload on a bad config, so `systemctl reload` becomes the single command that drives a full safe binary upgrade. Quick start now uses `systemctl reload pg_doorman.service` accordingly.
- `Restart=always` instead of `on-failure`: the production expectation is that the pool never stays down on the host, even after an explicit `systemctl stop`.
- `LimitNOFILE=65536` instead of `1048576`: the 1M default was over-cautious; 65536 covers most OLTP pools and the comment now explains how to size it from `pool_size * num_pools` plus clients.
- `User=postgres` / `Group=postgres` instead of a dedicated `pg_doorman` account: many deployments already have the postgres account, reusing it keeps file ownership aligned with PostgreSQL.
- `SyslogIdentifier=pg-pooler`: matches the production journal identifier so `journalctl -u pg_doorman -t pg-pooler` is the documented lookup path.
- `ExecStop=/bin/kill -SIGTERM $MAINPID`: redundant with the default but explicit makes the stop path obvious next to the reload path.
- Drop `KillMode=mixed`: the production setup runs without it and works because Type=notify already updates MainPID during the handoff window; keeping it pulled out lines up with the deployed unit. Mention `MemoryMax/Nice/CPUAffinity` as orthogonal resource controls in a trailing paragraph.

Operational checklist and step 3 of Quick start updated to match. EN and RU edited in lock-step.
@vadv
vadv merged commit e4365b5 into master May 13, 2026
51 checks passed
@vadv
vadv deleted the fix/bdd-web-ui-quoting branch May 13, 2026 13:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant